Conversation
The function assumed that `User.get_by_type_and_id` returns a User instance or None. However, this called method raises a `helios_auth.models.User.DoesNotExist` exception. This commit changes the behavior of `create_user` to match the behavior of `User.get_by_type_and_id`.
Example$ ./manage.py shell
Python 3.10.6 (main, Nov 14 2022, 16:10:14) [GCC 11.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> import django
>>> from helios_auth.models import User
>>> from helios_auth.auth_systems.password import create_user
>>> django.VERSION
(2, 2, 24, 'final', 0)
>>> create_user("andi","very-secret-password", "Andi")
Traceback (most recent call last):
File "<console>", line 1, in <module>
File "/home/andi/workspace/helios-server/helios_auth/auth_systems/password.py", line 24, in create_user
user = User.get_by_type_and_id('password', username)
File "/home/andi/workspace/helios-server/helios_auth/models.py", line 48, in get_by_type_and_id
return cls.objects.get(user_type = user_type, user_id = user_id)
File "/home/andi/virtualenvs/helios/lib/python3.10/site-packages/django/db/models/manager.py", line 82, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/home/andi/virtualenvs/helios/lib/python3.10/site-packages/django/db/models/query.py", line 406, in get
raise self.model.DoesNotExist(
helios_auth.models.User.DoesNotExist: User matching query does not exist.Note: I only tested this with Python 3.10 and Django 2.21 Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the create_user function that incorrectly assumed User.get_by_type_and_id returns None when a user doesn't exist. The method actually raises a User.DoesNotExist exception (as confirmed by the Django ORM's objects.get() behavior), which would have caused the function to crash instead of properly handling existing users.
Key Changes:
- Replaced conditional check with try/except/else block to properly handle
User.DoesNotExistexception - Aligned error handling pattern with other usages in the same file (lines 65, 90)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if user: | ||
|
|
||
| try: | ||
| User.get_by_type_and_id('password', username) |
There was a problem hiding this comment.
The result of User.get_by_type_and_id is not assigned to any variable. While the current code works correctly for checking existence, it would be more idiomatic to assign it to a variable (e.g., user = User.get_by_type_and_id('password', username)) for consistency with other usages in the codebase (see lines 65 and 90).
| User.get_by_type_and_id('password', username) | |
| user = User.get_by_type_and_id('password', username) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The function assumed that
User.get_by_type_and_idreturns a User instance or None. However, this called method raises ahelios_auth.models.User.DoesNotExistexception.This commit changes the behavior of
create_userto match the behavior ofUser.get_by_type_and_id.